-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Auto imports not working properly with RxJS 7 beta #43034
Comments
Yes, first you need to import something from The issue in the other files (where it hasn't been manually imported), then VSCode suggests My environment, btw:
|
Reproduced for me with TS 4.2.3. Yes, There is no suggestions to import After adding mentioned import {} from 'rxjs/operators' OS Version: Code Version:
All extensions are disabled. |
I've been digging a bit around TSServer and am I correct the fix might be in moduleSpecifiers.ts#tryGetModuleNameAsNodeModule? As far as I can see, that function receives a declaration file path (e.g. Would it be possible to update this file to also consider subfolders? |
This is caused by the highly unusual package organization present in RxJS 7. |
This issue has been marked as 'External' and has seen no recent activity. It has been automatically closed for house-keeping purposes. |
@benlesh let me know if you can’t work out a way to get a good UX given this information and we can brainstorm solutions. Don’t want the closing to come off as dismissive; it just doesn’t seem like this is a bug on our end right now. |
I wonder if it can be inferred in some way... The root's RxJS@7 package.json has
When we use Now, if we use In a way, this map feels like it should be simple:
Although I understand that it's not as straightforward. Otherwise is there any suggestion on how to change the structure to support these kind of imports? |
There’s no guarantee it had to work out that way. The relative paths match up because it made sense to a human to do it that way, but I could fork RxJS and make a version that breaks that expectation.
There are lots of solutions, because you really have to be doing non-standard stuff to break this in the first place. In general, shipping types in a different subfolder (e.g. |
@andrewbranch rxjs now has the |
@andrewbranch if the onus is on us to make sure that TypeScript/VSCode can find what it needs, that's fine. I just need to know what to do. How do we resolve this on our end? From our perspective, we have to support 3-4 module types, typescript files for source maps, typescript files for types, etc. It's all pretty nuts anyhow. It's not like there's a solid roadmap laid out for how to develop a library that meets everyone's needs. |
FWIW: I agree that the rxjs package is weird. It's ALL weird. I hate module resolution and I'll hate it until CJS dies or ESM gives up. |
What if we exported the types alongside the esm, (literally next to them in the same directories), but kept everything where it is. Does that solve the issue? |
In the future, yes, but right now, unfortunately not.
It at least solves part of the issue, and getting from there to a good UX should be fairly simple. The main issue here is that a module |
Well, looking more at your package structure, I might need to qualify more. TypeScript currently doesn’t have any way of distinguishing between separate entry points for CJS and ESM. So if you put all your typings alongside the ESM, the paths that we’re going to recognize are going to be Give that a try—if it doesn’t work, I’ll look at it myself next week. (Feel free to DM me on Twitter or in the TypeScript Community Discord.) |
@andrewbranch so basically something like this? "typesVersions": {
">=4.1": { "*": ["dist/types/*"] }
} |
This is an attempt to fix issues with VS Code auto-import, as described here in this issue: https://github.com/microsoft/TypeScript/issues/43034\#issuecomment-820621280 Related #6067
@andrewbranch This change is in This is the tsconfig: {
"compilerOptions": {
"allowSyntheticDefaultImports": true,
"experimentalDecorators": true,
"esModuleInterop": true,
"isolatedModules": true,
"strict": true,
"strictNullChecks": true,
"noImplicitAny": true,
"strictPropertyInitialization": true,
"strictBindCallApply": true,
"strictFunctionTypes": true,
"moduleResolution": "node",
"module": "esnext",
"noEmit": true,
"target": "esnext",
"jsx": "react",
"allowJs": true,
"typeRoots": [
"./types",
"./node_modules/@types"
],
"types": [
"jest",
"node"
],
"skipLibCheck": true,
"lib": [
"es2019",
"dom"
]
},
"exclude": [
"./docs",
"./env/"
]
} |
Here's a minimal reproduction. I'm not at all sure what to do about it. |
Your |
@andrewbranch that seems to work for |
Okay, I suppose what's the ideal setup? CJS next to types? ESM next to types? Assuming that we have to put all of the other stuff in other directories, and we don't want multiple copies of the types. |
Are you talking about module resolution or auto imports? I tested importing from |
Both, honestly. I couldn't get either working properly. |
That's the case even before the |
They were doing exactly what they were expected to do given the structure of the package at the time. Without
The screenshot I posted shows the changes I made to your package.json from 7.0.0-rc.1 alongside module resolution for |
So I installed After that, when I try to use I'm sure I'm just misunderstanding what needs done. Sorry, @andrewbranch I'm not trying to be frustrating. haha. |
See this comment above:
|
It may feel that way, but we are making progress! What you’re seeing now is also expected and can be fixed on your end. @voliva had it right:
Which is the same thing that I was referencing here:
The problem you’re seeing right now is that (The same topic was also discussed in #30033 (comment)) |
Can you think of a way I can test this in an automated way? |
Literally testing auto-imports functionality would be hard and out of scope, in my opinion. It should be trivial to test module resolution on a tiny TypeScript program with |
- Adds typesVersions, enforcing TypeScript >= 4.2. - Adds some superfluous imports required for TS and VS Code to find proper import locations for our strange deep import sites. - Independantly tested by building the package and installing it locally in another project, then testing in VS code. Fixes ReactiveX#6067 Related microsoft/TypeScript#43034
Thanks so much for your help, @andrewbranch! The solution did work to get things auto-importing in VS Code properly, however, ultimately we couldn't use this solution because of how it effected tree-shaking with bundlers like Rollup. You can read the fallout here: ReactiveX/rxjs#6276 We're going to have to figure out another solution, which is likely going to involve just getting rid of deep imports, as they've been nothing but a pain. |
@benlesh you can use /// <reference path="./operators/index.ts" /> instead of |
- Adds typesVersions, enforcing TypeScript >= 4.2. - Adds some superfluous references required for TS and VS Code to find proper import locations for our strange deep import sites. - Independantly tested by building the package and installing it locally in another project, then testing in VS code. Fixes ReactiveX#6067 Related microsoft/TypeScript#43034
- Adds typesVersions, enforcing TypeScript >= 4.2. - Adds some superfluous references required for TS and VS Code to find proper import locations for our strange deep import sites. - Independantly tested by building the package and installing it locally in another project, then testing in VS code. Fixes #6067 Related microsoft/TypeScript#43034
Disable Path Intellisense. That worked for me. |
Steps to Reproduce:
rxjs@7.0.0-beta.12
to a projectconcatMap
and check the possible auto-import locationsrxjs/operators
as the correct import location. Instead it showsrxjs/dist/types/operators
.Does this issue occur when all extensions are disabled?: Yes
This comes from an issue reported on the RxJS repository here: ReactiveX/rxjs#6067
Apparently, neovim users don't experience this issue, so it leads me to believe it's not the configuration of the package.
The text was updated successfully, but these errors were encountered: